-
Notifications
You must be signed in to change notification settings - Fork 427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix issue #68 editor with activeRow selection #241
Conversation
Great, #68 was a bug I looked at, and the more I looked the further it went. Sometimes these things are due to several different use cases inadvertently having conflicting requirements, and they can only be resolved by laying out the requirements for each explicitly and working out a way to accommodate them all. Glad this patch was able to resolve the issue :-) |
Glad you merged it since it was an issue in my current App development 👍 Will you consider that issue #68 closed then, or is that still a different bug? |
Will have to go back and repro the issue in #68. Some other time. |
I did use the example that was given in the issue to make the code change, which is why I had multiple commits. I couldn't reproduce (tested scenarios) after the code change that is now merged, which is why I was asking if we should just close the issue. |
Sure, if it's been tested and the original bug is resolved, absolutely close it. You should have the rights to do that? Happy for you to do so. |
Yeah I do, but I like to check with you anyway, team work ;) |
This fixes the error reported in issue #68 with a
Slick.RowSelectionModel({selectActiveRow: true})
was not triggering properly the Editor.What was happening is that the Editor was getting triggered but canceled out by the row selection. The fix is to set the
setActiveCellInternal
function 4th argument toTrue
(that arg issuppressActiveCellChangedEvent
). This makes the Editor to work correctly and doesn't trigger the cell active change. Also we want to do this only when our Grid is Editable.@6pac
Totally sorry about the last commit made on Master, I was supposed to be in my own bugfix branch but forgot to create the branch before pushing the commit. So I reverted the change... Again sorry about that.
Tested Scenarios
selectActiveRow
to FalseautoEdit: false
grid.setSelectionModel(new Slick.RowSelectionModel({selectActiveRow: false}));
selectActiveRow
to TrueautoEdit: false
grid.setSelectionModel(new Slick.RowSelectionModel({selectActiveRow: true}));
selectActiveRow
to FalseautoEdit: true
grid.setSelectionModel(new Slick.RowSelectionModel({selectActiveRow: false}));
selectActiveRow
to TrueautoEdit: true
grid.setSelectionModel(new Slick.RowSelectionModel({selectActiveRow: true}));
Extra Commits
After testing it intensively, I found out that the argument
suppressActiveCellChangedEvent
should only be set toTrue
when there is actually an Editor associated to the cell that we just clicked. This way it cancels out the Active Cell when Editing (which is what we want) but doesn't when there isn't any Editor attached.See below to describe what I wrote in previous paragraph.
The example below is set with
autoEdit: true
andselectActiveRow: true
, the grid has Column B editable but Column C is not. When we click to edit any cell of Column B, it won't activate the row, but as soon as we click on any row of Column C it will activate the row since there is not Editor associated to that cell. I believe this is the correct behavior.